Harden API and WebSocket receive paths - #1845
Draft
skot wants to merge 1 commit into
Draft
Conversation
This was referenced Aug 5, 2026
johnny9
reviewed
Aug 7, 2026
johnny9
left a comment
johnny9
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed exact head c340ca1. The bounded receive loops and WebSocket cap are sound, but I found one remaining JSON framing defect and a material test-coverage gap: none of the new receive/fragmentation/failure/boundary policies are exercised. I prepared two sequential patches; together they pass 77/77 ESP32-S3 QEMU tests and a fresh full ESP-IDF 5.5.3 firmware build.
| /* Respond with 500 Internal Server Error */ | ||
| httpd_resp_send_err(req, HTTPD_500_INTERNAL_SERVER_ERROR, "content too long"); | ||
| return ESP_OK; | ||
| esp_err_t receive_result = HTTP_receive_body(req, buf, SCRATCH_BUFSIZE); |
Collaborator
There was a problem hiding this comment.
Medium: HTTP_receive_body now supplies a correctly terminated buffer, but the handlers pass it to cJSON_Parse, which accepts a valid first JSON value followed by arbitrary non-whitespace bytes. Settings, pool, boot, and theme requests can therefore accept malformed documents such as {...} trailing. The proposal reply makes all four request parsers require the complete body.
Collaborator
There was a problem hiding this comment.
Proposed patch. It applies to the exact reviewed head; it was compile-validated in the full ESP-IDF 5.5.3 firmware build and included in the combined validation:
diff --git a/main/http_server/http_server.c b/main/http_server/http_server.c
index a789d35..7ca78f4 100644
--- a/main/http_server/http_server.c
+++ b/main/http_server/http_server.c
@@ -1078,7 +1078,7 @@ static esp_err_t PATCH_update_settings(httpd_req_t * req)
return ESP_FAIL;
}
- cJSON * root = cJSON_Parse(buf);
+ cJSON * root = cJSON_ParseWithOpts(buf, NULL, true);
if (root == NULL) {
httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Invalid JSON");
return ESP_OK;
@@ -1272,7 +1272,7 @@ static esp_err_t PUT_system_pool(httpd_req_t *req)
return ESP_FAIL;
}
- cJSON *root = cJSON_Parse(buf);
+ cJSON *root = cJSON_ParseWithOpts(buf, NULL, true);
if (!root) {
return httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Invalid JSON");
}
@@ -1453,7 +1453,7 @@ static esp_err_t POST_system_boot(httpd_req_t *req)
return ESP_FAIL;
}
- cJSON *root = cJSON_Parse(buf);
+ cJSON *root = cJSON_ParseWithOpts(buf, NULL, true);
if (!root) {
return httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Invalid JSON");
}
diff --git a/main/http_server/theme_api.c b/main/http_server/theme_api.c
index 7878ee8..f2d0b0d 100644
--- a/main/http_server/theme_api.c
+++ b/main/http_server/theme_api.c
@@ -57,7 +57,7 @@ static esp_err_t theme_post_handler(httpd_req_t *req)
return ESP_FAIL;
}
- cJSON *root = cJSON_Parse(content);
+ cJSON *root = cJSON_ParseWithOpts(content, NULL, true);
if (!root) {
httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Invalid JSON");
return ESP_FAIL;
| static int system_wifi_scan_prebuffer_len = 256; | ||
| static int api_common_prebuffer_len = 256; | ||
|
|
||
| esp_err_t HTTP_receive_body(httpd_req_t *req, char *buffer, size_t buffer_size) |
Collaborator
There was a problem hiding this comment.
Coverage: the new shared body reader and WebSocket cap have no automated tests. In particular, the exact buffer boundary, fragmented positive reads, zero/negative/over-read failures, and 1024/1025-byte WebSocket boundary are unprotected. The proposal reply extracts the decisions into a small production-used api_rx policy component and covers all of those branches in QEMU.
Collaborator
There was a problem hiding this comment.
Proposed coverage patch (apply after the strict-JSON patch above). It passes 77/77 ESP32-S3 QEMU tests and a fresh full ESP-IDF 5.5.3 firmware build:
diff --git a/components/api_rx/CMakeLists.txt b/components/api_rx/CMakeLists.txt
new file mode 100644
--- /dev/null
+++ b/components/api_rx/CMakeLists.txt
@@ -0,0 +1,4 @@
+idf_component_register(
+ SRCS "api_rx.c"
+ INCLUDE_DIRS "include"
+)
diff --git a/components/api_rx/include/api_rx.h b/components/api_rx/include/api_rx.h
new file mode 100644
--- /dev/null
+++ b/components/api_rx/include/api_rx.h
@@ -0,0 +1,21 @@
+#ifndef API_RX_H_
+#define API_RX_H_
+
+#include <stdbool.h>
+#include <stddef.h>
+
+#define API_RX_MAX_WEBSOCKET_PAYLOAD_SIZE 1024U
+
+typedef enum {
+ API_RX_BODY_READ_CONTINUE,
+ API_RX_BODY_READ_COMPLETE,
+ API_RX_BODY_READ_INVALID,
+} api_rx_body_read_result_t;
+
+bool api_rx_http_body_fits(size_t content_len, size_t buffer_size);
+api_rx_body_read_result_t api_rx_body_read_update(size_t content_len,
+ size_t *received_total,
+ int received);
+bool api_rx_websocket_payload_fits(size_t payload_len);
+
+#endif /* API_RX_H_ */
diff --git a/components/api_rx/api_rx.c b/components/api_rx/api_rx.c
new file mode 100644
--- /dev/null
+++ b/components/api_rx/api_rx.c
@@ -0,0 +1,30 @@
+#include "api_rx.h"
+
+bool api_rx_http_body_fits(size_t content_len, size_t buffer_size)
+{
+ return content_len > 0 && content_len < buffer_size;
+}
+
+api_rx_body_read_result_t api_rx_body_read_update(size_t content_len,
+ size_t *received_total,
+ int received)
+{
+ if (received_total == NULL || *received_total > content_len ||
+ received <= 0) {
+ return API_RX_BODY_READ_INVALID;
+ }
+
+ size_t received_size = (size_t)received;
+ if (received_size > content_len - *received_total) {
+ return API_RX_BODY_READ_INVALID;
+ }
+
+ *received_total += received_size;
+ return *received_total == content_len ? API_RX_BODY_READ_COMPLETE
+ : API_RX_BODY_READ_CONTINUE;
+}
+
+bool api_rx_websocket_payload_fits(size_t payload_len)
+{
+ return payload_len <= API_RX_MAX_WEBSOCKET_PAYLOAD_SIZE;
+}
diff --git a/components/api_rx/test/CMakeLists.txt b/components/api_rx/test/CMakeLists.txt
new file mode 100644
--- /dev/null
+++ b/components/api_rx/test/CMakeLists.txt
@@ -0,0 +1,5 @@
+idf_component_register(
+ SRCS "test_api_rx.c"
+ INCLUDE_DIRS "."
+ REQUIRES unity api_rx
+)
diff --git a/components/api_rx/test/test_api_rx.c b/components/api_rx/test/test_api_rx.c
new file mode 100644
--- /dev/null
+++ b/components/api_rx/test/test_api_rx.c
@@ -0,0 +1,48 @@
+#include <stdint.h>
+
+#include "api_rx.h"
+#include "unity.h"
+
+TEST_CASE("HTTP body size policy preserves terminator space", "[api_rx]")
+{
+ TEST_ASSERT_FALSE(api_rx_http_body_fits(0, 16));
+ TEST_ASSERT_TRUE(api_rx_http_body_fits(15, 16));
+ TEST_ASSERT_FALSE(api_rx_http_body_fits(16, 16));
+ TEST_ASSERT_FALSE(api_rx_http_body_fits(SIZE_MAX, 16));
+ TEST_ASSERT_FALSE(api_rx_http_body_fits(1, 0));
+}
+
+TEST_CASE("HTTP body read policy handles fragments and failures", "[api_rx]")
+{
+ size_t received_total = 0;
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_CONTINUE,
+ api_rx_body_read_update(5, &received_total, 2));
+ TEST_ASSERT_EQUAL_size_t(2, received_total);
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_COMPLETE,
+ api_rx_body_read_update(5, &received_total, 3));
+ TEST_ASSERT_EQUAL_size_t(5, received_total);
+
+ received_total = 0;
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_INVALID,
+ api_rx_body_read_update(5, &received_total, 0));
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_INVALID,
+ api_rx_body_read_update(5, &received_total, -1));
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_INVALID,
+ api_rx_body_read_update(5, &received_total, 6));
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_INVALID,
+ api_rx_body_read_update(5, NULL, 1));
+
+ received_total = 6;
+ TEST_ASSERT_EQUAL_INT(API_RX_BODY_READ_INVALID,
+ api_rx_body_read_update(5, &received_total, 1));
+}
+
+TEST_CASE("WebSocket payload limit has strict boundary", "[api_rx]")
+{
+ TEST_ASSERT_TRUE(api_rx_websocket_payload_fits(0));
+ TEST_ASSERT_TRUE(api_rx_websocket_payload_fits(
+ API_RX_MAX_WEBSOCKET_PAYLOAD_SIZE));
+ TEST_ASSERT_FALSE(api_rx_websocket_payload_fits(
+ API_RX_MAX_WEBSOCKET_PAYLOAD_SIZE + 1U));
+ TEST_ASSERT_FALSE(api_rx_websocket_payload_fits(SIZE_MAX));
+}
diff --git a/main/CMakeLists.txt b/main/CMakeLists.txt
index 6372448..32258d5 100755
--- a/main/CMakeLists.txt
+++ b/main/CMakeLists.txt
@@ -89,6 +89,7 @@ PRIV_REQUIRES
"vfs"
"esp_driver_i2c"
"tcp_transport"
+ "api_rx"
"esp_mm"
EMBED_FILES "http_server/recovery_page.html"
diff --git a/main/http_server/http_server.c b/main/http_server/http_server.c
index 1790342..c25edb0 100644
--- a/main/http_server/http_server.c
+++ b/main/http_server/http_server.c
@@ -45,6 +45,7 @@
#include "log_buffer.h"
#include "cjson_utils.h"
#include "utils.h"
+#include "api_rx.h"
static const char * TAG = "http_server";
static const char * CORS_TAG = "CORS";
@@ -81,17 +82,18 @@ esp_err_t HTTP_receive_body(httpd_req_t *req, char *buffer, size_t buffer_size)
}
const size_t content_len = req->content_len;
- if (content_len == 0 || content_len >= buffer_size) {
+ if (!api_rx_http_body_fits(content_len, buffer_size)) {
return ESP_ERR_INVALID_SIZE;
}
size_t received_len = 0;
while (received_len < content_len) {
int received = httpd_req_recv(req, buffer + received_len, content_len - received_len);
- if (received <= 0) {
+ api_rx_body_read_result_t result = api_rx_body_read_update(
+ content_len, &received_len, received);
+ if (result == API_RX_BODY_READ_INVALID) {
return ESP_FAIL;
}
- received_len += (size_t)received;
}
buffer[received_len] = '\0';
diff --git a/main/http_server/websocket.c b/main/http_server/websocket.c
index b473299..f1b70fa 100644
--- a/main/http_server/websocket.c
+++ b/main/http_server/websocket.c
@@ -10,9 +10,9 @@
#include "websocket_api.h"
#include "http_server.h"
#include "log_buffer.h"
+#include "api_rx.h"
#define WS_LOG_SCRATCH_SIZE 2048
-#define WS_RX_MAX_PAYLOAD_SIZE 1024
static const char * TAG = "websocket";
@@ -220,12 +220,12 @@ esp_err_t websocket_handler(httpd_req_t *req)
// WebSocket stream synchronized. Never allocate based on a peer-provided
// frame length.
if (ws_pkt.len > 0) {
- if (ws_pkt.len > WS_RX_MAX_PAYLOAD_SIZE) {
+ if (!api_rx_websocket_payload_fits(ws_pkt.len)) {
ESP_LOGW(TAG, "Rejecting oversized WebSocket frame: %zu bytes", ws_pkt.len);
return ESP_ERR_INVALID_SIZE;
}
- uint8_t buf[WS_RX_MAX_PAYLOAD_SIZE];
+ uint8_t buf[API_RX_MAX_WEBSOCKET_PAYLOAD_SIZE];
ws_pkt.payload = buf;
return httpd_ws_recv_frame(req, &ws_pkt, sizeof(buf));
}
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index e98ced1..d972cfb 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -10,7 +10,7 @@ set(EXTRA_COMPONENT_DIRS "../components")
# - when invoking CMake directly: cmake -D TEST_COMPONENTS="xxxxx" ..
# - when using idf.py: idf.py -T xxxxx build
#
-set(TEST_COMPONENTS "stratum asic" CACHE STRING "List of components to test")
+set(TEST_COMPONENTS "stratum asic api_rx" CACHE STRING "List of components to test")
include($ENV{IDF_PATH}/tools/cmake/project.cmake)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden inbound HTTP API and WebSocket handling against oversized, fragmented, and malformed client input.
size_t, require space for the terminating NUL, and reject zero or oversized JSON bodies.Why
PATCH /api/systempreviously narrowed the network-controlledreq->content_lenfromsize_ttoint. On ESP32, a value such as4294967294becomes-2, bypasses the upper-bound check, and reachesbuf[total_len] = '\0', producing an out-of-bounds write before a request body is required.Several JSON handlers also called
httpd_req_recv()only once, so ordinary TCP fragmentation could produce partial JSON parsing and leave request data unread. The WebSocket handler allocatedframe_length + 1bytes without an application-level maximum, allowing a client to consume heap or leave the stream unsynchronized after allocation failure.Validation
Build
idf.py buildgit diff --checkHardware testing
Flashed the resulting firmware over USB to a physical bitaxeGamma 601 with one BM1370 and monitored the serial console throughout the tests.
The miner booted normally, joined Wi-Fi, reached its configured 525 MHz frequency, connected to CKPool, received new work, and continued submitting accepted shares during and after the malformed API traffic. At the end of the test it was hashing at approximately 1.09 TH/s with 29 accepted shares, zero rejected shares, approximately 7.65 MB free heap, and no panic, watchdog reset, allocation failure, or unexpected reboot in the serial log.
Live API cases exercised against the device included:
4294967294were safely rejected; the device remained healthy and the next system-info request returned HTTP 200.0xffffffffwas rejected and its client session was removed.ESP-IDF limitation
ESP-IDF v5.5.1 stores the protocol's 64-bit WebSocket payload length in the ESP32's 32-bit
size_t. This change enforces a strict application limit on the parsed value and removes all peer-sized allocation, but an encoded value above 32 bits that truncates to a small value cannot be distinguished in application code. Rejecting that form before narrowing requires a corresponding ESP-IDF parser change.